Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(selector): select based on kork's RequestContext #1038

Merged
merged 4 commits into from
Jan 31, 2020

Conversation

dreynaud
Copy link
Contributor

Let's consolidate the interfaces for OrcaServiceSelector and ClouddriverServiceSelector behind a single select() method. Callers no longer need to pass a selectorKey or gate's RequestContext, everything is now going through kork's RequestContext which is backed by AuthenticatedRequest.

As a result gate's RequestContext is now completely unused and we can get rid of it (as well as the deprecated methods in the service selectors).

@@ -66,16 +66,14 @@ public ArtifactService(

public List<Map> getArtifactCredentials(String selectorKey) {
return mapListCommand(
"artifactCredentials",
clouddriverServiceSelector.select(selectorKey)::getArtifactCredentials)
"artifactCredentials", clouddriverServiceSelector.select()::getArtifactCredentials)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectorKey is no longer used (prob could go in a separate PR to clean those up..) (same in a few below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this PR was already on the big side, might do an opportunistic cleanup later

@@ -44,13 +44,13 @@ class CertificateService {

List<Map> getCertificates(String selectorKey) {
listCommand("certificates") {
clouddriverServiceSelector.select(selectorKey).getCertificates()
clouddriverServiceSelector.select().getCertificates()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in places where we used to pass the selectorKey will the selection behavior change? do we care? (I assume the selector was always the app which is not in the authcontext)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the selector was the "user origin" app, which is in the auth context. You can get the same behavior as before with a config like this:

config:
  selectorClass: com.netflix.spinnaker.kork.web.selector.ByUserOriginSelector
  origin: deck

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the clouddriverServiceSelector factory method does the translation, so you get opted into the new selector if you had the previous style of configuration:

dynamicEndpoints:
  deck: url

HystrixFactory.newMapCommand(GROUP, "deleteTask") {
orcaServiceSelector.withContext(requestContext)deleteTask(id)
orcaServiceSelector.select()deleteTask(id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
orcaServiceSelector.select()deleteTask(id)
orcaServiceSelector.select().deleteTask(id)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, groovy... sigh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my! thanks

@@ -39,14 +39,14 @@ class DataService {

void getStaticData(String id, Map<String, String> filters, OutputStream outputStream) {
voidCommand("static", {
def response = clouddriverServiceSelector.select(null).getStaticData(id, filters)
def response = clouddriverServiceSelector.select().getStaticData(id, filters)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is interesting where we explicitly didn't want to select in the past but now we do. Probably not an issue, but curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I'd say eventually we want every call to go through a common selector for consistent behavior

Copy link
Contributor

@marchello2000 marchello2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minus that 1 groovy compilation issue, lgtm

@dreynaud dreynaud added the ready to merge Approved and ready for merge label Jan 31, 2020
@mergify mergify bot merged commit ba97e4d into spinnaker:master Jan 31, 2020
@mergify mergify bot added the auto merged label Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants